Skip to content

Add subcmd add#914

Merged
ken-matsui merged 2 commits intocabinpkg:mainfrom
SunPodder:add_cmd
Feb 25, 2024
Merged

Add subcmd add#914
ken-matsui merged 2 commits intocabinpkg:mainfrom
SunPodder:add_cmd

Conversation

@SunPodder
Copy link
Copy Markdown
Contributor

No description provided.

@SunPodder SunPodder marked this pull request as draft February 24, 2024 02:02
Copy link
Copy Markdown
Member

@ken-matsui ken-matsui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your time to work on this! I put several comments for performance and style consistency.

Comment thread src/Cmd/Add.cc Outdated
Comment thread src/Cmd/Add.cc Outdated
Comment thread src/Cmd/Add.cc Outdated
Comment thread src/Cmd/Add.cc Outdated
Comment thread src/Cmd/Add.hpp Outdated
Comment thread src/Cmd/Add.cc Outdated
Comment thread src/Cmd/Add.cc Outdated
Comment thread src/Cmd/Add.hpp Outdated
Comment thread src/Cmd/Add.cc Outdated
Comment thread src/Cmd/Add.cc Outdated
@SunPodder SunPodder marked this pull request as ready for review February 24, 2024 11:54
@SunPodder SunPodder requested a review from ken-matsui February 24, 2024 11:55
Copy link
Copy Markdown
Member

@ken-matsui ken-matsui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your time again! Just a few things. Can you please also deal with the clang-tidy error?

Comment thread src/Cli.cc Outdated
Comment thread src/Cmd/Add.cc Outdated
Comment thread src/Cmd/Add.cc Outdated
Comment thread src/Cmd/Add.cc Outdated
Comment thread src/Cmd/Add.cc Outdated
Comment thread src/Cmd/Add.cc Outdated
Comment thread src/Cmd/Add.cc Outdated
Comment thread src/Cmd/Add.cc Outdated
Comment thread src/Cmd/Add.cc Outdated
@SunPodder
Copy link
Copy Markdown
Contributor Author

All fixed including the clang-tidy error

@ken-matsui
Copy link
Copy Markdown
Member

ken-matsui commented Feb 24, 2024

@SunPodder Sorry, there are more clang-tidy errors. Could you please address them? (After that, LGTM)

Comment thread src/Cmd/Add.cc Outdated
Comment thread src/Cmd/Add.cc Outdated
Comment thread src/Cmd/Add.cc Outdated
Comment thread src/Cmd/Add.cc Outdated
Comment thread src/Cmd/Add.cc Outdated
Comment thread src/Cmd/Add.cc Outdated
Allows users to add project dependencies to poac.toml from the commandline
@SunPodder
Copy link
Copy Markdown
Contributor Author

image

Sorry, it must be annoying for you reviewing the same mistakes. clang-tidy throws errors on my machine even for existing codes which are correct and passes on ci. So it's hard to make sure everything is fine :)

@ken-matsui
Copy link
Copy Markdown
Member

No worries! Which version of clang-tidy are you using?

@SunPodder
Copy link
Copy Markdown
Contributor Author

No worries! Which version of clang-tidy are you using?

clang-tidy-15

@ken-matsui
Copy link
Copy Markdown
Member

It seems that clang-tidy-17 fixed the problem, just fyi: llvm/llvm-project#46097.

Copy link
Copy Markdown
Member

@ken-matsui ken-matsui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution!

@ken-matsui ken-matsui merged commit 3c352da into cabinpkg:main Feb 25, 2024
@ken-matsui
Copy link
Copy Markdown
Member

@SunPodder Can you also update README.md for this command?

@SunPodder
Copy link
Copy Markdown
Contributor Author

Sure

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants